Knowage 8043 #891

BojanSovticEngIT · 2023-09-06T14:59:13Z

Files changed:

knowage-core/src/main/java/it/eng/spagobi/analiticalmodel/document/service/ManagePreviewFileAction.java - Should we remove FileUtils.checkPathTraversalAttack?
knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java
knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java
knowage-core/src/main/java/it/eng/spagobi/sdk/utilities/SDKObjectsConverter.java
knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/BirtImageServlet.java - Shoud we remove custom Path traversal check inside this method?
knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/utilities/Utils.java
knowagecockpitengine/src/main/java/it/eng/knowage/engine/cockpit/api/export/excel/ExcelExporter.java
knowagecommonjengine/src/main/java/it/eng/spagobi/engines/commonj/runtime/WorksRepository.java
knowagejasperreportengine/src/main/java/it/eng/spagobi/engines/jasperreport/JasperReportRunner.java
knowagemeta/src/main/java/it/eng/knowage/meta/generator/jpamapping/JpaMappingClassesGenerator.java
knowagetalendengine/src/main/java/it/eng/spagobi/engines/talend/runtime/JobDeploymentDescriptor.java - Method is not being used anywhere, we can only check file name I think?
knowagetalendengine/src/main/java/it/eng/spagobi/engines/talend/runtime/RuntimeRepository.java
knowageutils/src/main/java/it/eng/spagobi/tools/dataset/utils/datamart/DefaultEngineDatamartRetriever.java
knowageutils/src/main/java/it/eng/spagobi/utilities/SpagoBIAccessUtils.java
knowageutils/src/test/java/it/eng/spagobi/security/utils/PasswordEncryptionToolController.java

This reverts commit 6f40ce8.

Files changed (all)

davide-zerbetto

Changes are required allover modified files

davide-zerbetto · 2023-09-13T09:22:22Z

...e/src/main/java/it/eng/spagobi/analiticalmodel/document/service/ManagePreviewFileAction.java

+		try {
+			PathTraversalChecker.get(fileName, targetDirectory.getName());
+		} catch (Exception e) {
+			throw new PathTraversalAttackException("Error managing preview file for file: " + fileName);
+		}


PathTraversalChecker should throw only PathTraversalAttackException exception, that is runtime exception, so we can remove the try catch bloch

davide-zerbetto · 2023-09-13T09:22:49Z

...e/src/main/java/it/eng/spagobi/analiticalmodel/document/service/ManagePreviewFileAction.java

+			PathTraversalChecker.get(fileName, targetDirectory.getName());
+		} catch (Exception e) {
+			throw new PathTraversalAttackException("Error managing preview file for file: " + fileName);
+		}
 		FileUtils.checkPathTraversalAttack(fileName, targetDirectory);


Remove this method and use PathTraversalChecker instead

davide-zerbetto · 2023-09-13T09:24:29Z

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

-
-		java.nio.file.Path outputPath = Paths.get(SpagoBIUtilities.getResourcePath(), "dossier", String.valueOf(documentId), fileName);
-		File file = outputPath.toFile();
+		String outPath = SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator + fileName;


Use PathTraversalChecker.get method:
PathTraversalChecker.get(SpagoBIUtilities.getResourcePath() + separator + "dossier" , documentId, fileName)

davide-zerbetto · 2023-09-13T09:27:48Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

+			throw new PathTraversalAttackException(
+					"Error getting removind/moving dataset file for orginal file " + originalFileName + " and new file " + newFileName);
+		}


Remove this part

davide-zerbetto · 2023-09-13T09:28:45Z

knowageutils/src/main/java/it/eng/knowage/commons/security/PathTraversalChecker.java

@@ -37,6 +37,25 @@ private PathTraversalChecker() {
 		throw new IllegalStateException("This class provides utility methods. It cannot be instantiated");
 	}

+	public static void get(String firstDirectory, String... otherFolders) throws PathTraversalAttackException {


Rename firstDirectory as "trustedDirectory" or "safeDirectory"

davide-zerbetto · 2023-09-13T09:30:18Z

knowageutils/src/main/java/it/eng/knowage/commons/security/PathTraversalChecker.java

+			} catch (Exception e) {
+				logger.error(e);
+				throw new PathTraversalAttackException(e.getMessage());
+			}


If the code is not throwing any exception (apart from runtime PathTraversalAttackException), we can remove the try catch block

davide-zerbetto · 2023-09-13T09:30:44Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

+		} catch (Exception e) {
+			throw new PathTraversalAttackException("Error deleting dataset file " + fileName);
+		}


remove this catch block

Requested pull changes done. (Removed try/catch for PathTraversalChecker methods, renamed PathTraversalChecker.get property name.

davide-zerbetto · 2023-09-18T09:20:10Z

...e/src/main/java/it/eng/spagobi/analiticalmodel/document/service/ManagePreviewFileAction.java

@@ -107,7 +108,7 @@ public void doService() {
 	// checks for path traversal attacks
 	private void checkRequiredFile(String fileName) {
 		File targetDirectory = GeneralUtilities.getPreviewFilesStorageDirectoryPath();
-		FileUtils.checkPathTraversalAttack(fileName, targetDirectory);
+		PathTraversalChecker.get(fileName, targetDirectory.getName());


it doesn't make sense: inputs are reversed, since targetDirectory is the safe dir and filename is what has to be checked

davide-zerbetto · 2023-09-18T09:23:02Z

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

 		JSONObject response = new JSONObject();
 		File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator);
 		try {
-			PathTraversalChecker.isValidFileName(fileName);
-			PathTraversalChecker.preventPathTraversalAttack(file, dossierDir);
+			PathTraversalChecker.get(fileName, dossierDir.getName());


inputs reversed

davide-zerbetto · 2023-09-18T09:23:14Z

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

@@ -171,8 +169,7 @@ public Response checkPathFile(@QueryParam("templateName") String fileName, @Quer
 		File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator);
 		JSONObject response = new JSONObject();
 		try {
-			PathTraversalChecker.isValidFileName(fileName);
-			PathTraversalChecker.preventPathTraversalAttack(file, dossierDir);
+			PathTraversalChecker.get(fileName, dossierDir.getName());


inputs reversed

davide-zerbetto · 2023-09-18T09:26:38Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

 		File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase());
-		if (originalDatasetFile.exists()) {
+
+		if (newDatasetFile != null && originalDatasetFile != null && originalDatasetFile.exists()) {


I don't see how files can be null

davide-zerbetto · 2023-09-18T09:26:51Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

 		File datasetFile = new File(filePath + fileName);
-		if (datasetFile.exists()) {
+
+		if (datasetFile != null && datasetFile.exists()) {


I don't see how files can be null

davide-zerbetto · 2023-09-18T09:27:43Z

knowage-core/src/main/java/it/eng/spagobi/sdk/utilities/SDKObjectsConverter.java

+				PathTraversalChecker.isValidFileName(fileName);
+				File attachment = new File(fileName);
+
+				if (attachment != null && attachment.exists() && attachment.isFile()) {


I don't see how files can be null

davide-zerbetto · 2023-09-18T09:32:18Z

knowage-core/src/main/java/it/eng/spagobi/sdk/utilities/SDKObjectsConverter.java

-				File attachment = new File(dh.getName());
-				if (attachment.exists() && attachment.isFile()) {
+				String fileName = dh.getName();
+				PathTraversalChecker.isValidFileName(fileName);


fileName seems to be a complete path instead of a single file name. Please check

We need to talk about this in today's call.

revert the changes, most likely this is a false positive

davide-zerbetto · 2023-09-18T09:40:21Z

knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/BirtImageServlet.java

-				logger.error("File " + imageFile.getPath() + " not found");
-				return;
-			}
+			PathTraversalChecker.get(parent.toString(), fileName);


parent is NOT a safe dir, imageDirectory or imageTmpDir are safe.
Filename is a complete path, don't see how PathTraversalChecker.get will work

We need to talk about this in today's call.

Added required pull changes

Fix

davide-zerbetto · 2023-09-18T12:49:36Z

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

+		PathTraversalChecker.get(safeDirectory, fileName);
+		String outPath = safeDirectory + separator + fileName;


Merge them in 1 line: PathTraversalChecker.get has to return a File or something similar

davide-zerbetto · 2023-09-18T12:50:09Z

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

 		ResponseBuilder responseBuilder = null;
-
+		File file = new File(outPath);


merge this with the above

davide-zerbetto · 2023-09-18T12:57:25Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

+		PathTraversalChecker.get(filePath, originalFileName);
 		File originalDatasetFile = new File(filePath + originalFileName);


merge those lines

davide-zerbetto · 2023-09-18T12:57:36Z

knowage-core/src/main/java/it/eng/spagobi/api/SelfServiceDataSetCRUD.java

+		PathTraversalChecker.get(fileNewPath, newFileName + "." + fileType.toLowerCase());
 		File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase());


merge those lines

davide-zerbetto · 2023-09-18T13:15:34Z

knowage-core/src/main/java/it/eng/spagobi/sdk/utilities/SDKObjectsConverter.java

-				File attachment = new File(dh.getName());
-				if (attachment.exists() && attachment.isFile()) {
+				String fileName = dh.getName();
+				PathTraversalChecker.isValidFileName(fileName);


revert the changes, most likely this is a false positive

davide-zerbetto · 2023-09-18T13:40:07Z

knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/utilities/Utils.java

+		PathTraversalChecker.get(fileName, directory);
+		htmlFile = new File(directory, fileName);


merge those 2 lines

use
File htmlFile = PathTraversalChecker.get(BirtReportServlet.OUTPUT_FOLDER, reportExecutionId, fileName);

davide-zerbetto · 2023-09-18T13:58:08Z

knowagecommonjengine/src/main/java/it/eng/spagobi/engines/commonj/runtime/WorksRepository.java

+		PathTraversalChecker.get(rootDir.getName(), fileName);
+		File workDir = new File(rootDir, fileName);


merge those 2 lines

Changed preventPathTraversalAttack method to private, fixed in other files except DossierActivityResource.

Updated PathTraversalChecker.get

Updated PathTraversalChecker.get inside importTemplateFile method

Fixed htmlFile = PathTraversalChecker.get

Chages reverted, false positive

Fix

Fixes

Moved PathTraversal check to the calling function in excel exporter

PathTraversal fixes

Added docs

Changes with Davide Z. (Meet)

knowageutils/src/test/java/it/eng/spagobi/security/utils/PasswordEncryptionToolController.java

knowageutils/src/main/java/it/eng/spagobi/utilities/SpagoBIAccessUtils.java

...rc/main/java/it/eng/spagobi/tools/dataset/utils/datamart/DefaultEngineDatamartRetriever.java

knowagetalendengine/src/main/java/it/eng/spagobi/engines/talend/runtime/RuntimeRepository.java

knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/BirtImageServlet.java

knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java

knowage-core/src/main/java/it/eng/spagobi/api/DocumentExecutionResource.java

...jasperreportengine/src/main/java/it/eng/spagobi/engines/jasperreport/JasperReportRunner.java

Changes from the meet

Fix

BojanSovticEngIT added 3 commits August 29, 2023 16:50

Revert "Possible Bugfix KNOWAGE-8043"

da96b6a

This reverts commit 6f40ce8.

Merge remote-tracking branch 'upstream/master' into KNOWAGE-8043

997e9ab

Bugfix KNOWAGE-8043

e3c6a3c

Files changed (all)

davide-zerbetto added the security Security related issues label Sep 13, 2023

Merge remote-tracking branch 'upstream/master' into KNOWAGE-8043

aaa3d09

davide-zerbetto suggested changes Sep 13, 2023

View reviewed changes

Bugfix KNOWAGE-8043

8a66b78

Requested pull changes done. (Removed try/catch for PathTraversalChecker methods, renamed PathTraversalChecker.get property name.

BojanSovticEngIT requested a review from davide-zerbetto September 13, 2023 10:00

davide-zerbetto suggested changes Sep 18, 2023

View reviewed changes

BojanSovticEngIT added 3 commits September 18, 2023 12:32

Bugfixes KNOWAGE-8043

09ea635

Added required pull changes

Update DossierActivityResource.java

fd75ee0

Fix

Update SelfServiceDataSetCRUD.java

c560439

Fix

davide-zerbetto suggested changes Sep 18, 2023

View reviewed changes

BojanSovticEngIT added 12 commits September 19, 2023 10:19

Bugfix KNOWAGE-8043

0fab752

Changed preventPathTraversalAttack method to private, fixed in other files except DossierActivityResource.

Update WorksRepository.java

2284ba0

Updated PathTraversalChecker.get

Update DossierActivityResource.java

f302cdb

Updated PathTraversalChecker.get inside importTemplateFile method

Update Utils.java

b168ec2

Fixed htmlFile = PathTraversalChecker.get

Update SDKObjectsConverter.java

9bb7574

Chages reverted, false positive

Update SelfServiceDataSetCRUD.java

71231ad

Fix

Update DossierActivityResource.java

b7ecf4e

Fixes

Fixes

c0dc7cc

Moved PathTraversal check to the calling function in excel exporter

PathTraversal fixes

f0f1db5

PathTraversal fixes

Update PathTraversalChecker.java

45a0eff

Added docs

Merge remote-tracking branch 'upstream/master' into KNOWAGE-8043

6520e49

Bugfix Knowage-8043

ca94b0d

Changes with Davide Z. (Meet)

davide-zerbetto suggested changes Sep 20, 2023

View reviewed changes

BojanSovticEngIT added 2 commits September 20, 2023 13:09

Bugfix Knowage-8043

fe68d3d

Changes from the meet

Update ManagePreviewFileAction.java

6d9fa38

Fix

davide-zerbetto merged commit dc6c9b1 into KnowageLabs:master Sep 20, 2023
1 check passed

github-actions bot locked and limited conversation to collaborators Sep 20, 2023

		PathTraversalChecker.get(safeDirectory, fileName);
		String outPath = safeDirectory + separator + fileName;

		ResponseBuilder responseBuilder = null;

		File file = new File(outPath);

		PathTraversalChecker.get(filePath, originalFileName);
		File originalDatasetFile = new File(filePath + originalFileName);

		PathTraversalChecker.get(fileNewPath, newFileName + "." + fileType.toLowerCase());
		File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase());

		PathTraversalChecker.get(fileName, directory);
		htmlFile = new File(directory, fileName);

		PathTraversalChecker.get(rootDir.getName(), fileName);
		File workDir = new File(rootDir, fileName);

Knowage 8043 #891

Knowage 8043 #891

Conversation

BojanSovticEngIT commented Sep 6, 2023

davide-zerbetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment